Repository navigation
Conversation
A mounted Lightbox keeps its video element after the dialog closes. The playback effect paused only a gallery neighbour, so Close, Escape, or a tap outside left the sound running. Closing now pauses every video. Opening again plays the current one when hasAutoPlay is set, including a single video that has no page. The new close cases failed before the effect watched isOpen. vitest run packages/core/src/Lightbox/Lightbox.test.tsx --project ui passed 91 tests after the fix.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hi @SebTardif! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
github-actions
Bot
added
community
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner's decision, not on changes from you.
- The Lightbox spec covers pausing a gallery video on a neighbouring page while another item shows (FR18), but says nothing about what a closed viewer does with playback, so an engineering owner needs to confirm that closing should pause a playing video where it stopped (and that reopening with
hasAutoPlayplays it on).
The new close and reopen tests fail on main and pass here, and in Chromium playback continues behind the closed dialog on main and stops on this branch. CI has not run on this head yet.
Optional, non-blocking:
- The added comment sentence "Opening again plays the current one when
hasAutoPlayis set." restates the code below it and could go. - Not caused by this pull request: a Lightbox mounted closed with
hasAutoPlaystill plays its video behind the closed dialog; renderingautoPlay={isOpen && isCurrent && hasAutoPlay}would close that gap, since the effect already plays the current video on open.
[Automated review]
The closed dialog still mounts the video. hasAutoPlay set the autoPlay attribute even when isOpen was false, so a viewer mounted closed started playback once the file loaded. The attribute now follows isOpen. The close comment no longer restates the play check. The new attribute test failed before this change because the closed viewer rendered autoplay. The playback tests passed after it.
|
The two optional notes are in 43c7d2a. The close comment no longer restates the play check, and The owner decision is unchanged. FR18 covers a video on a neighbouring page. This pull request still needs an engineering owner to confirm that closing pauses playback. |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision, not on changes from you.
- The Lightbox spec covers pausing a gallery video on a neighbouring page while another item shows (FR18), but says nothing about what a closed viewer does with playback, so an engineering owner needs to confirm that closing pauses a playing video where it stopped, that a viewer mounted closed with
hasAutoPlaywaits until it opens, and that reopening withhasAutoPlayplays the current video on.
Both earlier optional notes are addressed: the effect comment keeps only the reason, and autoPlay now follows isOpen. The four new tests fail on main and pass here, and in Chromium playback stops on close on this branch while it continues behind the closed dialog on main. CI has not run on this head yet; a maintainer adds the ci:run label so it runs.
[Automated review]
meta-cla
Bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
User impact
Someone watching a video in Lightbox hears it keep playing after they close the viewer. Close, Escape, and a tap outside leave the component mounted. The picture is hidden and the sound continues.
Problem and solution fit
The playback effect pauses a gallery video only when that video is no longer the current page. Closing does not change the current page, and a single video is not on a page, so nothing calls
pause. The closed dialog keeps the element, and a hidden video keeps its audio. The effect now pauses every video while the viewer is closed, and plays the current one again on open whenhasAutoPlayis set.Expected behavior and authority
Gallery paging already stops a video that leaves the screen, and plays it again with
hasAutoPlay(Lightbox FR18). Closing hides that same mounted element. Autoplay still followshasAutoPlay.Smallest restoration
The existing playback effect now also depends on
isOpen. While closed it pauses every video and returns. While open it keeps the neighbour pause and plays the current video, including a single video that has no page element. No new prop and no visual change.Evidence
pnpm exec vitest run packages/core/src/Lightbox/Lightbox.test.tsx --project ui -t "pauses a playing video when the viewer closes"failed becausepausewas called 0 times. The video element was still in the document.hasAutoPlaystill plays that video.pauses a playing video when the viewer closes,pauses a single playing video when the viewer closes, andplays a single video again when the viewer reopens, with hasAutoPlayinLightbox.test.tsx.Scope
Testing
pnpm exec vitest run packages/core/src/Lightbox/Lightbox.test.tsx --project ui(91 passed). The new cases mockHTMLMediaElement.pauseandplayin jsdom, the same way the existing paging tests observe playback.The neighbour pause landed with the gallery pager in #8197. This covers the dismiss path.